Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

P0619R4 Removing C++17-Deprecated Features #380

Merged
merged 12 commits into from
Feb 3, 2020

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Dec 11, 2019

Description

This implements #28 by conditionally removing deprecated features when compiling in C++20 mode following the guidance from P0619R4

Checklist

Be sure you've read README.md and understand the scope of this repo.

If you're unsure about a box, leave it unchecked. A maintainer will help you.

  • Identifiers in product code changes are properly _Ugly as per
    https://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
  • The STL builds successfully and all tests have passed (must be manually
    verified by an STL maintainer before automated testing is enabled on GitHub,
    leave this unchecked for initial submission).
  • These changes introduce no known ABI breaks (adding members, renaming
    members, adding virtual functions, changing whether a type is an aggregate
    or trivially copyable, etc.).
  • These changes were written from scratch using only this repository,
    the C++ Working Draft (including any cited standards), other WG21 papers
    (excluding reference implementations outside of proposed standard wording),
    and LWG issues as reference material. If they were derived from a project
    that's already listed in NOTICE.txt, that's fine, but please mention it.
    If they were derived from any other project (including Boost and libc++,
    which are not yet listed in NOTICE.txt), you must mention it here,
    so we can determine whether the license is compatible and what else needs
    to be done.

@miscco miscco requested a review from a team as a code owner December 11, 2019 15:41
@miscco miscco changed the title Implement P0619R4 Remove deprecated features Dec 11, 2019
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect we will need escape hatch macros for these, at least:

  • an escape hatch that brings back the extra typedefs
  • an escape hatch that brings back shared_ptr::unique
  • an escape hatch that brings back is_pod
  • an escape hatch that brings back result_of

stl/inc/type_traits Outdated Show resolved Hide resolved
@CaseyCarter
Copy link
Member

I suspect we will need escape hatch macros for these, at least:

  • an escape hatch that brings back the extra typedefs
  • an escape hatch that brings back shared_ptr::unique
  • an escape hatch that brings back is_pod
  • an escape hatch that brings back result_of

I'd prefer to see a single escape hatch in the style of _HAS_AUTO_PTR_ETC.

Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be missing at least uncaught_exception; please audit P0619R4 to ensure you haven't missed other removals.

@BillyONeal
Copy link
Member

I suspect we will need escape hatch macros for these, at least:

  • an escape hatch that brings back the extra typedefs
  • an escape hatch that brings back shared_ptr::unique
  • an escape hatch that brings back is_pod
  • an escape hatch that brings back result_of

I'd prefer to see a single escape hatch in the style of _HAS_AUTO_PTR_ETC.

Our experience with _HAS_AUTO_PTR_ETC is precisely why that scares me -- looking at the number of RWC projects that need that does not fill me with confidence that such a granular switch will ever be disabled by large projects.

Then again I'm on record saying the standard shouldn't ever remove anything :)

@barcharcraz
Copy link
Member

if we continue to deprecate and remove stuff from the standard but not the code it may make sense to have something like _CXX17_DEPRECATE_ADAPTOR_TYPEDEFS and friends that hard errors on their use (or removes them from the headers).

@StephanTLavavej
Copy link
Member

Having proposed WG21-N4190 and implemented _HAS_AUTO_PTR_ETC, I now believe that having a single mega-escape-hatch is counterproductive. That is, it should have been _HAS_AUTO_PTR, _HAS_RANDOM_SHUFFLE, and _HAS_OLD_FUNCTIONAL_STUFF. Note that we have fine-grained escape hatches for the C++17 removals P0003R5 Removing Dynamic Exception Specifications (_HAS_UNEXPECTED) and P0004R1 Removing Deprecated Iostreams Aliases (_HAS_OLD_IOSTREAMS_MEMBERS), which (happily!) are virtually never activated because those things are obscure.

I think that following the design we've been using for deprecations would be optimal - that is, fine-grained escape hatches, which are also controlled by an overall escape hatch. The naming for the overall escape hatch could be verbose like _HAS_FEATURES_REMOVED_BY_CXX20.

(Separately, we could retrofit the C++17 removal macros accordingly, but I'd want to see that as a followup change, and no action might be necessary there.)

@CaseyCarter
Copy link
Member

I now believe that having a single mega-escape-hatch is counterproductive

I think having only a coarse-grained hatch encourages faster migration. Folks who don't care about deprecation/removal also don't care about coarse vs. fine - they'll just hit the big red button and go about their business either way. Folks who do care about avoiding dependencies on deprecated/removed features will eradicate all uses from local code, and only use escape hatches for third-party code. If we give them fine-grained hatches, they will enable only those necessary for their third-party code to compile thereby reducing their exposure. If we give them only coarse-grained hatches, they will have to accept maximum exposure to use any third-party code that uses a deprecated/removed feature. Being more dissatisfied in the latter case encourages the motivated users to push harder on unmotivated authors of third-party code.

That said, I am motivated just enough by this question to type this explanation - I won't die on this hill, but I'll yell loudly as I retreat ;)

@StephanTLavavej
Copy link
Member

We still have to enable _HAS_AUTO_PTR_ETC for many Real World Code projects, despite the size of the option. I understand your argument, but it seems unlikely that "motivated users" would notice the size of a coarse-grained escape hatch - how many users realized what ETC controlled?

My argument is straightforward - we use a fine+coarse combo for deprecations, following that for removals seems consistent.

@miscco
Copy link
Contributor Author

miscco commented Dec 12, 2019

I started implementing the changes, but for adaptor typedefs it is a bit more work.

@miscco
Copy link
Contributor Author

miscco commented Dec 13, 2019

I added escape hatches to retain the removed features. I hope that this is what was expected

Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have not double checked the paper or attempted the tests yet but this looks reasonable

stl/inc/ostream Outdated
@@ -109,7 +109,11 @@ public:
_STL_DISABLE_DEPRECATED_WARNING
__CLR_OR_THIS_CALL ~sentry() noexcept {
#if _HAS_EXCEPTIONS
#if _HAS_DEPRECATED_UNCAUGHT_EXCEPTION
Copy link
Member

@BillyONeal BillyONeal Dec 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still blocked by ArchivedOS-12000909. @amyw-msft Do you know what the status of that is?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any update here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows doesn't build in C++20 mode, and won't start trying until C++20 is fully implemented, so I think we're unblocked here. (Also, the Windows build can simply define the fine-grained escape hatch macro.)

stl/inc/xmemory Outdated Show resolved Hide resolved
@AdamBucior
Copy link
Contributor

But is_pod hasn't been removed yet. Shouldn't it at least be available by default?

stl/inc/type_traits Outdated Show resolved Hide resolved
stl/inc/type_traits Outdated Show resolved Hide resolved
stl/inc/type_traits Outdated Show resolved Hide resolved
stl/inc/exception Outdated Show resolved Hide resolved
stl/inc/functional Outdated Show resolved Hide resolved
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
@miscco
Copy link
Contributor Author

miscco commented Jan 22, 2020

I will try to have a look on it this evening

@miscco
Copy link
Contributor Author

miscco commented Jan 22, 2020

I addressed most review comments. As allocator<void> was the only one without an escape hat I added one too for good measure.

There are some remaining comments from @BillyONeal that I would like to wait on.

I did not silence the warnings when somebody retains the removed features. They should carry that cost.

Also one stylistic question remains. Currently I use _HAS_DEPRECATED_FOO. Should it rather be _HAS_REMOVED_FOO?

@StephanTLavavej
Copy link
Member

I did not silence the warnings when somebody retains the removed features. They should carry that cost.

Agreed! Also consistent with the auto_ptr precedent.

Also one stylistic question remains.

That's a good question - of those two, I prefer _HAS_REMOVED_MEOW, because it counteracts the removal, and it makes these macros visually distinct from the deprecation silencing.

However, I worry a little bit about grammatical ambiguity - the intent is "has meow, a removed feature in C++20" (i.e. "removed" is an adjective), but it could also be read as "this macro has removed (verb) meow", which is the opposite sense. The older macros _HAS_AUTO_PTR_ETC, _HAS_UNEXPECTED, _HAS_OLD_IOSTREAMS_MEMBERS, etc. didn't say REMOVED, so they weren't affected by such ambiguity.

So, two more ideas:

  1. Continue to follow that precedent and simply use _HAS_MEOW: so _HAS_RESULT_OF, _HAS_UNCAUGHT_EXCEPTION, etc.
  2. Use _HAS_OLD_MEOW (originally used to describe the "old iostreams members"), thereby indicating that old machinery is being restored. _HAS_OLD_RESULT_OF, _HAS_OLD_UNCAUGHT_EXCEPTION, _HAS_OLD_NEGATORS (I really like that one because they are distinct from the new not_fn tech).

The more I think about it, the more I like _HAS_OLD_MEOW. It avoids the contradiction in saying that we "have" a "removed" thing, but also indicates that something old and yucky is being requested, so it's mild discouragement.

stl/inc/xmemory Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter self-requested a review January 22, 2020 22:21
@miscco
Copy link
Contributor Author

miscco commented Jan 23, 2020

I did not silence the warnings when somebody retains the removed features. They should carry that cost.

Agreed! Also consistent with the auto_ptr precedent.

Also one stylistic question remains.

That's a good question - of those two, I prefer _HAS_REMOVED_MEOW, because it counteracts the removal, and it makes these macros visually distinct from the deprecation silencing.

However, I worry a little bit about grammatical ambiguity - the intent is "has meow, a removed feature in C++20" (i.e. "removed" is an adjective), but it could also be read as "this macro has removed (verb) meow", which is the opposite sense. The older macros _HAS_AUTO_PTR_ETC, _HAS_UNEXPECTED, _HAS_OLD_IOSTREAMS_MEMBERS, etc. didn't say REMOVED, so they weren't affected by such ambiguity.

So, two more ideas:

  1. Continue to follow that precedent and simply use _HAS_MEOW: so _HAS_RESULT_OF, _HAS_UNCAUGHT_EXCEPTION, etc.
  2. Use _HAS_OLD_MEOW (originally used to describe the "old iostreams members"), thereby indicating that old machinery is being restored. _HAS_OLD_RESULT_OF, _HAS_OLD_UNCAUGHT_EXCEPTION, _HAS_OLD_NEGATORS (I really like that one because they are distinct from the new not_fn tech).

The more I think about it, the more I like _HAS_OLD_MEOW. It avoids the contradiction in saying that we "have" a "removed" thing, but also indicates that something old and yucky is being requested, so it's mild discouragement.

I fully concur with your reasoning. However I believe that indeed _HAS_DEPRECATED_MEOW is the better choice than _HAS_OLD_MEOW as the latter is more or less neutral in meaning.

In contrast _HAS_DEPRECATED_MEOW tells you that you are using something that even the C++ commitee thought so broken that it removed it from the language.

@miscco
Copy link
Contributor Author

miscco commented Jan 23, 2020

I dropped the allocator patch due to @BillyONeal comment

@BillyONeal BillyONeal self-requested a review January 28, 2020 02:10
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!

This looks good to me provided @StephanTLavavej (the deprecations and removals czar) and @amyw-msft (for ArchivedOS-12000909 ) are OK with it.

This updates llvm-project as the escape hatch macro
has been checked in upstream.

This updates skipped_tests.txt because non-Standard code
has appeared in the to_array test.
@BillyONeal BillyONeal changed the title Remove deprecated features P0619R4 Removing C++17-Deprecated Features Feb 1, 2020
@StephanTLavavej StephanTLavavej merged commit b350426 into microsoft:master Feb 3, 2020
@StephanTLavavej
Copy link
Member

Thank you for helping encourage the C++ community to modernize their codebases! This has passed our internal testing and has been merged for VS 2019 16.6 Preview 2. We’ll need to report issues in various “Real World Code” projects (e.g. Boost) this week as our extended test suites run.

@StephanTLavavej
Copy link
Member

Also filed Dolphin-11970 "fmt 5.3.0 is incompatible with C++20" (fixed in fmt 6.0.0).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants